Conversation
…5751) Chained_Collection_Assertions_WithStrings called .Count(c => c.IsEqualTo(3)) on string[]. Before #5751 this failed to bind to the per-item filter overload (int-vs-string mismatch on IsEqualTo) and resolved to the inline-count assertion form, returning a chainable assertion. After #5751 added IsEqualTo<TValue, TOther> with implicit-conversion fallback, both overloads compile and resolution picks the per-item filter, which returns CollectionCountSource<,> -- a count source without .And -- breaking .And.Contains("Bob") with CS1061. Mirror the int test (Chained_Collection_Assertions) and use .Count().IsEqualTo(3) so the resulting assertion is the chainable CollectionCountEqualsAssertion regardless of TItem.
thomhurst
had a problem deploying
to
Pull Requests
GitHub Actions
Failure
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review of PR #5759: Fix Chained_Collection_Assertions_WithStrings compilation failure
This is a clean, minimal fix for the regression introduced by PR #5751. The root cause analysis is accurate and the chosen fix is the right one. Here is my assessment:
What the fix does
The test previously relied on an implicit compile error to steer overload resolution toward the inline-count form Count(Func<IAssertionSource<int>, Assertion<int>?>). PR #5751 introduced IsEqualTo<TValue, TOther> at [OverloadResolutionPriority(-1)], which made c.IsEqualTo(3) against IAssertionSource<string> compile (via the implicit-conversion fallback). This changed overload resolution so the compiler now picks Count(Func<IAssertionSource<TItem>, IAssertion?>) — the per-item filter overload — which returns CollectionCountSource<string[], string>, a builder object that has no .And.
The fix switches to the .Count().IsEqualTo(3) form, which routes through CollectionCountSource<TCollection, TItem>.IsEqualTo(int) → CollectionCountEqualsAssertion<TCollection, TItem>, a proper CollectionAssertionBase subclass that does expose .And. This is consistent with how Chained_Collection_Assertions (the int-array variant) already works.
Design observations
The fix itself is correct. There are no production-code changes, only a test correction. The 2-line change is the minimal, targeted solution.
The underlying design tension is worth noting (not a blocker for this PR). There are now three Count overloads on CollectionAssertionBase:
Count() → CollectionCountSource (numeric builder)
Count(Func<IAssertionSource<int>, Assertion<int>?>) → CollectionCountWithInlineAssertionAssertion (priority -1)
Count(Func<IAssertionSource<TItem>, IAssertion?>) → CollectionCountSource (per-item filter)
When TItem = int, both predicate overloads are applicable for a lambda like c => c.IsEqualTo(5), and the [OverloadResolutionPriority(-1)] on the inline-count form correctly deprioritises it. But as this PR demonstrates, the interaction between these overloads and the new IsEqualTo<TValue, TOther> implicit-conversion overload can be surprising — the compiler now silently picks the per-item filter form for non-int collections when the lambda happens to compile via the implicit-conversion fallback. A future improvement could be a Roslyn analyzer diagnostic that warns when Count(predicate) is used in a way that returns a CollectionCountSource without a subsequent numeric assertion, flagging the likely intent mismatch.
The updated comment in the test is accurate and helpful. Explaining that Count(predicate) is a per-item filter that returns a builder without .And provides future readers with the context needed to choose the right form.
Correctness
- The replaced expression
.Count().IsEqualTo(3)correctly counts the total number of elements (no predicate → simpleICollection.Countfast-path) and chains back to aCollectionAssertionBase, so.And.Contains(...)and.And.DoesNotContain(...)work as intended. - No production code is changed.
- There is no risk to AOT compatibility or dual-mode (source-gen + reflection) operation.
Verdict
LGTM. The fix is minimal, correct, and consistent with the existing pattern in the sibling test. Merging this unblocks main without any behavioural regression.
Up to standards ✅🟢 Issues
|
1 participant
Summary
CS1061: 'CollectionCountSource<string[], string>' does not contain a definition for 'And'inTUnit.Assertions.Tests/CollectionAssertionTests.cs:270.feat(assertions): IsEqualTo with implicitly-convertible wrappers) addedIsEqualTo<TValue, TOther>with an implicit-conversion fallback. That madec.IsEqualTo(3)againstIAssertionSource<string>compile, where it previously didn't. The test relied on that earlier compile error to push overload resolution to the inline-count assertion form (Count(Func<IAssertionSource<int>, Assertion<int>?>)). Now resolution picks the per-item filter overloadCount(Func<IAssertionSource<TItem>, IAssertion?>)which returnsCollectionCountSource<,>— a builder with no.And.Chained_Collection_Assertions) already uses:.Count().IsEqualTo(3). The result isCollectionCountEqualsAssertion(aCollectionAssertionBase), so.Andis available regardless of the item type.No production code change; behaviour of
.Count(itemAssertion)and.IsEqualTo<,>is unchanged.Test plan
dotnet build TUnit.Assertions.Tests -c Release→ 0 errors locallydotnet run --framework net10.0 -- --treenode-filter "/*/*/CollectionAssertionTests/Chained_Collection_Assertions_WithStrings*"→ 1/1 passed.NETworkflow on PR turns green (windows / ubuntu / macos)